Add Cholesky factorisation #202

AlexRobson · 2022-08-03T14:09:37Z

Presently Cholesky isn't supported with NamedDims.

Adding this was made fairly easy with the NamedFactorisation. It throws a dimensionamismatch if the axes are not named the same. Also includes the suggested linting changes from JuliaFormatter for some lines that were not part of this MR.

src/functions_linearalgebra.jl

test/functions_linearalgebra.jl

codecov · 2022-08-03T14:19:35Z

Codecov Report

Merging #202 (585c946) into master (f803d70) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   94.71%   94.79%   +0.07%     
==========================================
  Files          14       14              
  Lines         662      672      +10     
==========================================
+ Hits          627      637      +10     
  Misses         35       35

Impacted Files	Coverage Δ
src/functions_linearalgebra.jl	`96.34% <100.00%> (+0.50%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

AlexRobson · 2022-08-03T14:10:29Z

src/functions_linearalgebra.jl

+    n1, n2 = L
+    @assert n1 == n2 # Must be identical given square pdmat?
+    if d === :L
+        return NamedDimsArray{(n1, n2)}(inner)


For cholesky, I assume that both the lower triangular and upper triangular being square matricies would correspond to the same axis.

Yes, that would be my assumption as well. That being said, there's nothing stopping someone from constructing a factorization type with a square matrix, but having different names. Should probably replace the @assert with a DimensionMismatch error though.

Could probably also rewrite this block as:

return d in (:L, :U) ? NamedDimsArray{(n1, n2)}(inner) : inner

I realised actually we don't want the dimcheck at the point of getproperty, but at the point of constructing the NamedFactoriztion. I've added a new constructor for this

AlexRobson · 2022-08-03T14:20:38Z

src/functions_linearalgebra.jl

@@ -26,6 +26,7 @@ Base.propertynames(named::NamedFactorization; kwargs...) = propertynames(parent(
 Base.iterate(named::NamedFactorization{L, T, <:LU}) where {L, T} = (named.L, Val(:U))
 Base.iterate(named::NamedFactorization{L, T, <:LQ}) where {L, T} = (named.L, Val(:Q))
 Base.iterate(named::NamedFactorization{L, T, <:SVD}) where {L, T} = (named.U, Val(:S))
+Base.iterate(named::NamedFactorization{L, T, <:Cholesky}) where {L, T} = (named.L, Val(:U))


IIUC this correctly, this should return:

r = rand(4,4); Σ = r' * r _l, _u = cholesky(Σ) cholesky(Σ).L == _l cholesky(Σ).U == _u

Compare with lu::

_l, _u = lu(Σ) lu(Σ).L == _l # True lu(Σ).U == _u # True

This is working on julia 1 (1.6 and 1.7) but failing on 1.3 and 1.5

I think that's fine. Though we should probably make this a minor release if you'd prefer to just pin the Julia version to 1.6 or higher.

test/functions_linearalgebra.jl

rofinn

Seems mostly fine apart from cleaning up the @assert and bumping the release for Julia >=1.6.

rofinn · 2022-08-03T16:42:08Z

src/functions_linearalgebra.jl

@@ -26,6 +26,7 @@ Base.propertynames(named::NamedFactorization; kwargs...) = propertynames(parent(
 Base.iterate(named::NamedFactorization{L, T, <:LU}) where {L, T} = (named.L, Val(:U))
 Base.iterate(named::NamedFactorization{L, T, <:LQ}) where {L, T} = (named.L, Val(:Q))
 Base.iterate(named::NamedFactorization{L, T, <:SVD}) where {L, T} = (named.U, Val(:S))
+Base.iterate(named::NamedFactorization{L, T, <:Cholesky}) where {L, T} = (named.L, Val(:U))


I think that's fine. Though we should probably make this a minor release if you'd prefer to just pin the Julia version to 1.6 or higher.

rofinn · 2022-08-03T16:52:19Z

src/functions_linearalgebra.jl

+    n1, n2 = L
+    @assert n1 == n2 # Must be identical given square pdmat?
+    if d === :L
+        return NamedDimsArray{(n1, n2)}(inner)


Yes, that would be my assumption as well. That being said, there's nothing stopping someone from constructing a factorization type with a square matrix, but having different names. Should probably replace the @assert with a DimensionMismatch error though.

rofinn · 2022-08-03T16:54:08Z

src/functions_linearalgebra.jl

+    n1, n2 = L
+    @assert n1 == n2 # Must be identical given square pdmat?
+    if d === :L
+        return NamedDimsArray{(n1, n2)}(inner)


Could probably also rewrite this block as:

return d in (:L, :U) ? NamedDimsArray{(n1, n2)}(inner) : inner

test/functions_linearalgebra.jl

src/functions_linearalgebra.jl

test/functions_linearalgebra.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

AlexRobson · 2022-08-04T09:40:55Z

test/functions_linearalgebra.jl

+    @test dimnames(x.L) == (:foo, :foo)
+    @test dimnames(x.U) == (:foo, :foo)
+
+    @test_throws DimensionMismatch cholesky(nda_mismatch)


now throw an error if there is a dim-mismatch.

AlexRobson · 2022-08-04T09:41:10Z

.github/workflows/CI.yml

@@ -30,11 +29,6 @@ jobs:
            arch: x86
          - os: windows-latest
            arch: x86
-        include:


drop julia < 1.6 in CI

AlexRobson · 2022-08-04T09:41:27Z

Project.toml

@@ -1,7 +1,7 @@
 name = "NamedDims"
 uuid = "356022a1-0364-5f58-8944-0da4b18d706f"
 authors = ["Invenia Technical Computing Corporation"]
-version = "0.2.49"
+version = "0.3.0"


major bump due to constrainting julia to 1.6 and above

in the future, this should be a patch bump to be non-breaking (since the package is pre-1.0) unless one intends to support backports: https://github.com/SciML/ColPrac/pull/20/files

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

AlexRobson · 2022-08-04T09:51:28Z

src/functions_linearalgebra.jl

+    n1, n2 = L
+    return d in (:L, :U) ? NamedDimsArray{(n1, n2)}(inner) : inner
+end
+function NamedFactorization{L}(fact::Cholesky{T}) where {L,T}


Define a new constructor to handle the dimension mismatch

rofinn

Still looks fine to me

Alex Robson added 2 commits August 3, 2022 15:04

Add Cholesky Factorisation

9353ecf

Bump Project

221ec84

github-actions bot reviewed Aug 3, 2022

View reviewed changes

AlexRobson commented Aug 3, 2022

View reviewed changes

rofinn self-requested a review August 3, 2022 16:31

rofinn approved these changes Aug 3, 2022

View reviewed changes

Alex Robson added 3 commits August 4, 2022 09:34

Julia 1.6 only and change to minor release

7b80bc3

DimensionMismatch at construction not at getproperty

0c36a9d

Add Dimensionmismatch test

83bdaaf

github-actions bot reviewed Aug 4, 2022

View reviewed changes

src/functions_linearalgebra.jl Outdated Show resolved Hide resolved

src/functions_linearalgebra.jl Outdated Show resolved Hide resolved

src/functions_linearalgebra.jl Show resolved Hide resolved

test/functions_linearalgebra.jl Outdated Show resolved Hide resolved

Alex Robson and others added 2 commits August 4, 2022 10:05

Drop Julia < 1.6

ce8769b

Apply suggestions from code review

78b180d

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

AlexRobson commented Aug 4, 2022

View reviewed changes

AlexRobson requested a review from rofinn August 4, 2022 09:42

AlexRobson changed the title ~~[WIP] Add Cholesky factorisation~~ Add Cholesky factorisation Aug 4, 2022

Apply suggestions from code review

585c946

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

AlexRobson commented Aug 4, 2022

View reviewed changes

rofinn approved these changes Aug 4, 2022

View reviewed changes

AlexRobson merged commit 234871f into master Aug 4, 2022

mzgubic mentioned this pull request Aug 10, 2022

Release HEAD as 0.2 since 0.3 was nonbreaking #205

Merged

AlexRobson mentioned this pull request Sep 23, 2022

Drop support for Julia 1.5 and older invenia/FeatureTransforms.jl#110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cholesky factorisation #202

Add Cholesky factorisation #202

AlexRobson commented Aug 3, 2022 •

edited

Loading

codecov bot commented Aug 3, 2022 •

edited

Loading

AlexRobson Aug 3, 2022

rofinn Aug 3, 2022

rofinn Aug 3, 2022

AlexRobson Aug 4, 2022

AlexRobson Aug 3, 2022

AlexRobson Aug 3, 2022 •

edited

Loading

rofinn Aug 3, 2022

rofinn left a comment

rofinn Aug 3, 2022

rofinn Aug 3, 2022

rofinn Aug 3, 2022

AlexRobson Aug 4, 2022

AlexRobson Aug 4, 2022

AlexRobson Aug 4, 2022

ericphanson Aug 5, 2022

AlexRobson Aug 4, 2022

rofinn left a comment

Add Cholesky factorisation #202

Add Cholesky factorisation #202

Conversation

AlexRobson commented Aug 3, 2022 • edited Loading

codecov bot commented Aug 3, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexRobson Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn left a comment

Choose a reason for hiding this comment

AlexRobson commented Aug 3, 2022 •

edited

Loading

codecov bot commented Aug 3, 2022 •

edited

Loading

AlexRobson Aug 3, 2022 •

edited

Loading